DM-43060: Let checkExistingOutputs work without a full butler #284
Conversation
| @@ -312,10 +312,6 @@ def checkExistingOutputs(self, quantum: Quantum, taskDef: TaskDef, limited_butle | |||
| RuntimeError | |||
| Raised if some outputs exist and some not. | |||
| """ | |||
| if not self.butler: | |||
| # Skip/prune logic only works for full butler. | |||
| return False | |||
There was a problem hiding this comment.
@andy-slac, do you recall why we made this shortcut? At first I thought it must be because LimitedButler didn't have pruneDatasets...but it does. So this is either something we should have done a while ago (but maybe thought we didn't need) or there's something deeper.
This is all coming up because @mfisherlevine now has a LimitedButler that wraps a another LimitedButler (which is actually a full one in this use case) and does in-memory caching, and he's got a use case for that to be able to clobber.
There was a problem hiding this comment.
This is all passing, but I'll wait to hear back from @andy-slac before merging anything.
There was a problem hiding this comment.
I think this was because back then we needed access to butler.datastore for implementation of this method and limited butler did not provide that property. We had extended limited butler interface since then, so it should be OK now to allow limited butler here.
|
The docstring for this method needs some cleaning up now that limited butler is not just for queries. Should the docstring say how the limited butler relates to self.butler as well? |
|
Thanks Tim. This was all done with Jim, and I don't think I actually know enough about this to write something correct/that will make you happy. Could you or Jim want to just suggest how you think it should read and I'll fix it up? |
I think the docstring for |
mfisherlevine
force-pushed
the
tickets/DM-43060
branch
2 times, most recently
from
3774a40
to
22237a3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #284 +/- ##
==========================================
- Coverage 87.44% 87.44% -0.01%
==========================================
Files 49 49
Lines 4469 4467 -2
Branches 771 770 -1
==========================================
- Hits 3908 3906 -2
Misses 408 408
Partials 153 153 ☔ View full report in Codecov by Sentry. |
mfisherlevine
force-pushed
the
tickets/DM-43060
branch
from
22237a3
to
dc4445a
Compare
run_err.yaml
Outdated
There was a problem hiding this comment.
Looks like some left-over from debugging? Should it be here?
There was a problem hiding this comment.
I'm pretty sure that started getting created by tests that don't clean up after themselves in a different ticket. @mfisherlevine, if you can spot what's making it and make it clean up, all the better, but just adding it to .gitignore instead of committing it is probably sufficient.
There was a problem hiding this comment.
There was a problem hiding this comment.
@eigerx , can you fix this on another ticket (so Merlin can just do .gitignore)? You should be able to find some other examples of using temporary-directory context managers to put files like this in a temporary location in tests, and clean up at the end.
There was a problem hiding this comment.
Ack, the perils of routinely typing git add -A in repos. I'll just remove it from this ticket, but also leave it out of the gitignore, so that it doesn't just get ignored. Then, when @eigerx fixes it on the other branch, it'll just go away cleanly without leaving something vestigial in the .gitignore.
There was a problem hiding this comment.
mfisherlevine
force-pushed
the
tickets/DM-43060
branch
from
dc4445a
to
cb614fc
Compare
Checklist
doc/changes